Strong branching fixings + dual cutoff#922
Strong branching fixings + dual cutoff#922aliceb-nv wants to merge 15 commits intoNVIDIA:release/26.04from
Conversation
aliceb-nv
added
non-breaking
|
/ok to test f95031d |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds pruning for fixed fractional variables; threads incumbent upper bound into strong/trial branching; applies post-strong-branching and reduced-cost bound tightening that may fix variables, re-run advanced-basis root LP solves, or return INFEASIBLE/OPTIMAL/TIME_LIMIT/NUMERICAL statuses. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 2488-2497: The bounds_strengthening call mutates original_lp_
without synchronization; wrap the call to bounds_strengthening and any immediate
subsequent mutations/use of original_lp_ (e.g., the bounds vectors and the
following prune_fixed_fractional_variables call) in the mutex_original_lp_ lock
to prevent races with external set_new_solution() — acquire mutex_original_lp_
before calling bounds_strengthening(...) and release it after the related
updates to original_lp_ complete.
In `@cpp/src/branch_and_bound/pseudo_costs.cpp`:
- Around line 85-87: The code currently treats dual::status_t::ITERATION_LIMIT
as a proven bound and calls compute_objective(child_problem, solution.x); remove
ITERATION_LIMIT from that condition so obj is computed only for proven statuses
(dual::status_t::OPTIMAL and dual::status_t::CUTOFF) and ensure any logic that
consumes obj (root fixings / cutoff deductions) is not executed when status ==
dual::status_t::ITERATION_LIMIT; update the condition around compute_objective
in pseudo_costs.cpp (the block handling child_problem, solution.x and status) to
skip objective materialization and subsequent bound/fathom logic for
ITERATION_LIMIT.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cpp/src/branch_and_bound/branch_and_bound.cppcpp/src/branch_and_bound/pseudo_costs.cppcpp/src/branch_and_bound/pseudo_costs.hppcpp/src/mip_heuristics/solver.cu
|
/ok to test c84928b |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 2513-2516: The log prints "from propagation" as num_fixed -
num_tightened which can be negative because num_fixed counts fixed fractionals
and num_tightened counts tighten operations; fix by computing an explicit
non-negative propagated count before logging (e.g., int num_from_propagation =
num_fixed - num_tightened; if (num_from_propagation < 0) num_from_propagation =
0) and use that variable in the settings_.log.printf call (reference
settings_.log.printf, num_fixed, num_tightened) so the log never shows a
negative "from propagation" value.
- Around line 2526-2556: The current handling of dual::status_t lp_status (from
dual_phase2_with_advanced_basis) treats any non-OPTIMAL/TIME_LIMIT result as
NUMERICAL; change this to explicitly handle WORK_LIMIT and ITERATION_LIMIT (and
any other non-fatal statuses) instead of mapping them to
mip_status_t::NUMERICAL. Locate the lp_status checks around the root solve
(variable lp_status, compute_objective, root_relax_soln_) and the later re-solve
site, and: 1) add explicit branches for dual::status_t::WORK_LIMIT and
dual::status_t::ITERATION_LIMIT that propagate a corresponding solver_status_
(or return a distinct mip_status_t like WORK_LIMIT/ITERATION_LIMIT) OR use the
current objective (root_objective_) as a valid bound for branching/cutoff logic
before continuing; 2) only treat truly numerical failures as
mip_status_t::NUMERICAL. Ensure set_final_solution/set_solution_at_root behavior
is updated to match the new status paths.
| settings_.log.printf( | ||
| "Strong branching bounds tightening: %d variables fixed (%d from propagation)\n", | ||
| num_fixed, | ||
| num_fixed - num_tightened); |
There was a problem hiding this comment.
num_fixed - num_tightened can go negative in the log output.
Line 2516 mixes different counters (num_fixed = fixed fractionals; num_tightened = bound tighten operations), so the “from propagation” number can become negative and misleading.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/src/branch_and_bound/branch_and_bound.cpp` around lines 2513 - 2516, The
log prints "from propagation" as num_fixed - num_tightened which can be negative
because num_fixed counts fixed fractionals and num_tightened counts tighten
operations; fix by computing an explicit non-negative propagated count before
logging (e.g., int num_from_propagation = num_fixed - num_tightened; if
(num_from_propagation < 0) num_from_propagation = 0) and use that variable in
the settings_.log.printf call (reference settings_.log.printf, num_fixed,
num_tightened) so the log never shows a negative "from propagation" value.
|
🔔 Hi @anandhkb, this pull request has had no activity for 7 days. Please update or let us know if it can be closed. Thank you! If this is an "epic" issue, then please add the "epic" label to this issue. |
1 similar comment
|
🔔 Hi @anandhkb, this pull request has had no activity for 7 days. Please update or let us know if it can be closed. Thank you! If this is an "epic" issue, then please add the "epic" label to this issue. |
|
/ok to test b6a8980 |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cpp/src/branch_and_bound/branch_and_bound.cpp (1)
2642-2651:⚠️ Potential issue | 🟠 MajorHandle the actual dual-simplex termination statuses explicitly in these re-solves.
Everywhere else in this file treats LP infeasibility as
dual::status_t::DUAL_UNBOUNDED, so theseINFEASIBLEbranches are inconsistent, and the fallbackelsestill turnsWORK_LIMIT/ITERATION_LIMITintomip_status_t::NUMERICAL. That can abort after a non-fatal tightened-root solve even though the current bound is still usable in this module.Based on learnings: "When a solver terminates early with status ITERATION_LIMIT, the current objective value is still a valid (lower) bound for minimization, though looser than the optimum. Treat ITERATION_LIMIT results as safe for strong branching bound tightening, fixings, and cutoff logic in these modules."
Also applies to: 2707-2716
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/branch_and_bound/branch_and_bound.cpp` around lines 2642 - 2651, The lp re-solve branches must explicitly handle dual-simplex termination codes: replace checks for dual::status_t::INFEASIBLE with dual::status_t::DUAL_UNBOUNDED where appropriate (to match the rest of the file), and add explicit branches for dual::status_t::ITERATION_LIMIT and dual::status_t::WORK_LIMIT so they are treated as safe (use the current objective as a valid bound) instead of falling into the generic NUMERICAL path; for these cases update solver_status_ (e.g., to mip_status_t::TIME_LIMIT or a dedicated ITERATION_LIMIT status consistent with surrounding code), call set_final_solution(solution, root_objective_) if the code currently does that for early-termination cases, and return the corresponding solver_status_ rather than mip_status_t::NUMERICAL; apply the same changes to the analogous block around the other occurrence (the block covering the lines referenced 2707-2716).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 2603-2606: The code currently treats any infeasibility after
incumbent-based strong-branching (SB) or reduced-cost (RC) tightenings as model
infeasibility; instead, only return mip_status_t::INFEASIBLE when the deduction
is independent of the incumbent (i.e. num_cutoff == 0). Update the blocks that
check `feasible` (the SB branch around the `feasible` variable and the RC
propagation branches that may return NUMERICAL/INFEASIBLE) so that: (1) if the
infeasibility came from incumbent-based tightenings (all RC fixings and SB cases
with num_cutoff > 0) do not return INFEASIBLE/NUMERICAL but treat the outcome as
“no solution beats the incumbent”/incumbent-optimal, ensure the incumbent is
published before exiting; (2) only return INFEASIBLE when num_cutoff == 0 (true
model infeasibility); and (3) apply the same change at the other mentioned sites
(the other `feasible`/RC checks around the SB/RC propagation code paths) so all
exits uniformly collapse incumbent-based infeasible outcomes to
incumbent-optimal rather than reporting model infeasibility.
---
Duplicate comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 2642-2651: The lp re-solve branches must explicitly handle
dual-simplex termination codes: replace checks for dual::status_t::INFEASIBLE
with dual::status_t::DUAL_UNBOUNDED where appropriate (to match the rest of the
file), and add explicit branches for dual::status_t::ITERATION_LIMIT and
dual::status_t::WORK_LIMIT so they are treated as safe (use the current
objective as a valid bound) instead of falling into the generic NUMERICAL path;
for these cases update solver_status_ (e.g., to mip_status_t::TIME_LIMIT or a
dedicated ITERATION_LIMIT status consistent with surrounding code), call
set_final_solution(solution, root_objective_) if the code currently does that
for early-termination cases, and return the corresponding solver_status_ rather
than mip_status_t::NUMERICAL; apply the same changes to the analogous block
around the other occurrence (the block covering the lines referenced 2707-2716).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c25be81d-7b56-455b-8e49-c4103d86bde7
📒 Files selected for processing (2)
cpp/src/branch_and_bound/branch_and_bound.cppcpp/src/mip_heuristics/solver.cu
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp/src/mip_heuristics/solver.cu
|
/ok to test c5ebbb7 |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
cpp/src/branch_and_bound/branch_and_bound.cpp (2)
2665-2668:⚠️ Potential issue | 🟡 MinorHandle
WORK_LIMITandITERATION_LIMITexplicitly in SB re-solve path.The else branch treats all non-handled statuses as
NUMERICAL. Per learnings,ITERATION_LIMITprovides a valid dual bound andWORK_LIMITis a legitimate termination. Both should propagate their respective MIP statuses rather than being misclassified as numerical failures.📝 Suggested fix
} else if (lp_status == dual::status_t::TIME_LIMIT) { solver_status_ = mip_status_t::TIME_LIMIT; set_final_solution(solution, root_objective_); return solver_status_; + } else if (lp_status == dual::status_t::WORK_LIMIT) { + solver_status_ = mip_status_t::WORK_LIMIT; + set_final_solution(solution, root_objective_); + return solver_status_; + } else if (lp_status == dual::status_t::ITERATION_LIMIT) { + // Iteration limit during re-solve - treat as work limit + solver_status_ = mip_status_t::WORK_LIMIT; + set_final_solution(solution, root_objective_); + return solver_status_; } else { settings_.log.printf("LP re-solve after SB tightening returned status %d\n", lp_status); return mip_status_t::NUMERICAL; }Based on learnings: "In cuOpt, the dual simplex method is dual-feasible throughout execution… when status is ITERATION_LIMIT, the current objective is still a valid lower bound."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/branch_and_bound/branch_and_bound.cpp` around lines 2665 - 2668, The current else branch treats any unhandled lp_status as mip_status_t::NUMERICAL; instead detect solver statuses WORK_LIMIT and ITERATION_LIMIT (check lp_status against the solver's WORK_LIMIT and ITERATION_LIMIT constants) and return the corresponding mip_status_t values (e.g., mip_status_t::WORK_LIMIT and mip_status_t::ITERATION_LIMIT) while logging via settings_.log.printf; only fall back to mip_status_t::NUMERICAL for true numerical failures. Ensure you reference lp_status and replace the single return mip_status_t::NUMERICAL with an explicit conditional mapping to the proper mip_status_t enums.
2736-2739:⚠️ Potential issue | 🟡 MinorHandle
WORK_LIMITandITERATION_LIMITexplicitly in RC re-solve path.Same issue as the SB path: the else branch classifies all unhandled statuses as
NUMERICAL, butWORK_LIMITandITERATION_LIMITare valid termination conditions that should propagate their respective statuses.📝 Suggested fix
} else if (lp_status == dual::status_t::TIME_LIMIT) { solver_status_ = mip_status_t::TIME_LIMIT; set_final_solution(solution, root_objective_); return solver_status_; + } else if (lp_status == dual::status_t::WORK_LIMIT) { + solver_status_ = mip_status_t::WORK_LIMIT; + set_final_solution(solution, root_objective_); + return solver_status_; + } else if (lp_status == dual::status_t::ITERATION_LIMIT) { + solver_status_ = mip_status_t::WORK_LIMIT; + set_final_solution(solution, root_objective_); + return solver_status_; } else { settings_.log.printf("LP re-solve after RC tightening returned status %d\n", lp_status); return mip_status_t::NUMERICAL; }Based on learnings: "In cuOpt, the dual simplex method is dual-feasible throughout execution… when status is ITERATION_LIMIT, the current objective is still a valid lower bound."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/branch_and_bound/branch_and_bound.cpp` around lines 2736 - 2739, The else branch in the RC re-solve path currently maps any unhandled lp_status to mip_status_t::NUMERICAL; change it to explicitly handle WORK_LIMIT and ITERATION_LIMIT from the LP solver: check lp_status for WORK_LIMIT and return mip_status_t::WORK_LIMIT, check for ITERATION_LIMIT and return mip_status_t::ITERATION_LIMIT, otherwise keep the existing log + return mip_status_t::NUMERICAL (use the same settings_.log.printf context and lp_status variable in the messages). Ensure you update the branch that contains settings_.log.printf("LP re-solve after RC tightening returned status %d\n", lp_status); and the return mip_status_t::NUMERICAL; to perform these explicit checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 2665-2668: The current else branch treats any unhandled lp_status
as mip_status_t::NUMERICAL; instead detect solver statuses WORK_LIMIT and
ITERATION_LIMIT (check lp_status against the solver's WORK_LIMIT and
ITERATION_LIMIT constants) and return the corresponding mip_status_t values
(e.g., mip_status_t::WORK_LIMIT and mip_status_t::ITERATION_LIMIT) while logging
via settings_.log.printf; only fall back to mip_status_t::NUMERICAL for true
numerical failures. Ensure you reference lp_status and replace the single return
mip_status_t::NUMERICAL with an explicit conditional mapping to the proper
mip_status_t enums.
- Around line 2736-2739: The else branch in the RC re-solve path currently maps
any unhandled lp_status to mip_status_t::NUMERICAL; change it to explicitly
handle WORK_LIMIT and ITERATION_LIMIT from the LP solver: check lp_status for
WORK_LIMIT and return mip_status_t::WORK_LIMIT, check for ITERATION_LIMIT and
return mip_status_t::ITERATION_LIMIT, otherwise keep the existing log + return
mip_status_t::NUMERICAL (use the same settings_.log.printf context and lp_status
variable in the messages). Ensure you update the branch that contains
settings_.log.printf("LP re-solve after RC tightening returned status %d\n",
lp_status); and the return mip_status_t::NUMERICAL; to perform these explicit
checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 24b72020-5f2c-401f-83dd-a2650132a31f
📒 Files selected for processing (1)
cpp/src/branch_and_bound/branch_and_bound.cpp
|
/ok to test baf8f33 |
1 participant
This PR introduces fixings performed during the strong branching at the root if infeasible branches are detected.
The existing code always performs full strong branching on all fractionals at the root. If infeasibility is detected, we can safely conclude, for free, that:
We also incorporate the incumbent value into the strong branching cutoffs. This may enable tighter fixings as well if we can prove that certain branches cannot provide better incumbents than the cutoff.
Reduced cost strengthening is also now turned on with the default
cuopt_clisettings.This change introduces a -0.5% improvement in average on the dual bound, and tips
qap10to optimality within 10min on my setup.Description
Issue
Checklist